[intfsorch]: Fence new RIF dependencies during interface removal#4746
[intfsorch]: Fence new RIF dependencies during interface removal#4746Xichen96 wants to merge 2 commits into
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
22ec476 to
9951f52
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR fixes a race in the interface/VRF event flow that can cause an interface IP’s IP2Me (trap-to-CPU) /32 route to be programmed into the wrong virtual router when the interface is (re)bound to a non-default VRF. This is in the core SWSS control-plane path (cfgmgr → APP_DB INTF_TABLE → orchagent IntfsOrch → SAI route programming), and directly impacts correctness of return traffic (e.g., DHCP relay giaddr flows) in non-default VRFs.
Changes:
- Propagate the interface’s VRF binding into the IP-address
INTF_TABLEentry (vrf_name) so orchagent has an explicit target VRF for IP adds. - In
IntfsOrch::setIntf, defer installing the IP2Me route until the interface RIF’s current VR matches the configured VRF to avoid programming the route in a stale/previous VRF.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
orchagent/intfsorch.cpp |
Defers interface-IP IP2Me route creation until the RIF has moved to the configured VRF, preventing wrong-VRF route installs during VRF bind races. |
cfgmgr/intfmgr.cpp |
Adds vrf_name to IP-key INTF_TABLE entries by reading the interface VRF binding from STATE_DB, enabling correct VRF selection in orchagent. |
Comments suppressed due to low confidence (1)
orchagent/intfsorch.cpp:613
- The new VRF-mismatch deferral is a behavior change that should be covered by a regression test: when an interface IP add arrives while the RIF is still in the previous VRF,
setIntf()should return false (stay in toSync), and then succeed once the RIF is recreated in the target VRF (installing the IP2Me route in that VRF). Adding a focused mock test would prevent future refactors from reintroducing the race.
addIp2MeRoute(port.m_vr_id, *ip_prefix);
if(gMySwitchType == "voq")
{
if(gPortsOrch->isInbandPort(alias))
{
//Need to sync the inband intf neighbor for other asics
9951f52 to
79e73b5
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Refreshing the unchanged exact head for a full PR build; the previous regular vstest lane ended on unrelated full-suite flakes. |
|
/azp run |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
This PR has backport request label(s) for branch(es): 202605, but is missing required test information. Please make sure you tick the tested branch(es) in the Tested branch section and provide test evidence (e.g., 202605: <test result>) in the Test result section as well in your PR description. ---Powered by SONiC BuildBot
|
|
/azp run |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
How to read this two-PR fix
The easiest place to start is the externally visible failure in PR #4764: an explicit VRF-changing interface
SETcould be consumed while old prefixes still existed, so the RIF remained in the old VRF.Retaining that
SETis necessary but not sufficient. While the move waits for old dependencies to drain, RouteOrch and NeighOrch must not attach new work to the old RIF. This PR fixes that prerequisite dependency-drain race.The required implementation and merge order remains this PR first, then PR #4764, because the VRF-rehome fix builds on the explicit removal-state predicate introduced here.
What I did
Once deletion of an interface has started, new route and neighbor dependencies must not attach to the RIF being removed.
Root cause
Whole-interface deletion already had the correct retry model.
For these independent APP_DB keys:
the bare interface key is normally visited first because the consumer stores pending work in a key-ordered multimap. That ordering was safe:
This is why interface
DELarriving before prefixDELhad worked historically: deletion already checked its dependencies and retried.The deletion fence was incomplete, however:
DELinserted the alias intom_removingIntfses.DELalso erased that marker, even though the whole-interfaceDELwas still queued.getRouterIntfsId()accessor.DELremained blocked and could repeatedly lose the race to new work.The problem was not that deletion lacked retry. The problem was that other orchagents could refill the dependency count during that retry.
Fix
This PR completes the existing deletion state machine:
The implementation has three parts:
The whole-interface
DELowns the removal marker.Prefix deletion no longer clears
m_removingIntfses. The marker is cleared only when the whole-interface deletion succeeds.Addition paths explicitly check removal state.
Standard local route, neighbor, and neighbor-next-hop creation calls
isIntfRemovalPending()before obtaining the RIF. If removal is pending, the addition returnsfalse, leaving the original APP_DB operation in that orchagent'sm_toSyncqueue.The existing
getRouterIntfsId()accessor is unchanged. Existing dependencies and deletion paths must still find the old RIF so they can remove themselves and release its reference count. Guarding the getter itself would block both additions and deletions and deadlock the drain.RouteOrch honors deferred neighbor-next-hop creation.
Callers no longer request a next-hop ID immediately after
NeighOrch::addNextHop()reports that creation was deferred.The resulting execution is:
Why I did it
Why not enforce one global APP_DB order?
APP_DB StateTable is primarily a latest desired-state transport, not a durable transactional command log.
Even if one producer emits operations in the desired order, that does not create a global execution order because:
Replacing one multimap with a FIFO or priority queue would therefore order only one table. It would not order IntfsOrch against RouteOrch and NeighOrch, and a single blocked entry could introduce head-of-line blocking for unrelated interfaces.
The established SWSS design is instead:
This PR follows that design and adds a local per-interface admission fence. It does not claim to provide a transaction or generation protocol across all orchagents.
Scope
The guarded creation paths are the standard local interface route, neighbor, and neighbor-next-hop paths involved in the traced failure. Remote-system-port behavior is explicitly unchanged; its pre-existing inband RIF identity, bookkeeping, and boot-order issues are being handled independently.
How I verified it
IntfsOrchDeleteCreateRetrywith the real lexical sequence: whole-interfaceDEL, then prefixDEL.DELremains queued;LocalNeighborDependenciesRetryWhileInterfaceRemovalPendingto verify that local neighbor and next-hop additions return retry while the fence is active.1168368(master) and1168367(202605). Theirvstestjobs stopped before executing tests because those workers lackedpip3.1167872, passed traced and production 1x/5x runs and all ten production 10x test bodies. This PR is the smaller local deletion-fence subset; exact split-commit validation is provided by this PR's CI.Current exact-head master PR build
4c38411af466f770e8eb7282423443eed43fe872TestAsan vstest.Test vstestlane was not green after unrelated VNET, P4RT, and ACL full-suite flakes across retries. No failure in the focused interface-removal coverage was observed.202605 PR-build validation
202605, which already contains the shared PR-CI dependency fix.d4b0502ecdac76d56fdf063a633045c0fb0de977Tested for 202605 branchlabel must remain absent until that exact full-stack build is complete.Details if related